Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

chore: update license header #3623

Merged
merged 28 commits into from
Oct 1, 2024
Merged

chore: update license header #3623

merged 28 commits into from
Oct 1, 2024

Conversation

AJPfleger
Copy link
Contributor

@AJPfleger AJPfleger commented Sep 17, 2024 •
edited
Loading

Initial Problem

All files in our project have a different copyright header, which may or may not be updated when changes are done. This adds some clutter and unnecessary thinking to each PR.

Solution attempts

The most beautiful solution would be to automatically update each file with creation-date and when it was last touched. However, this is not possible, because of some reasons.

Creation date

We could go back into the git logs and check the first entry. This gets a bit confusing, when we rename/move files. In theory, it should work with --follow. However, I noticed, that often the suggested creation date is newer, than the actual one, following all PRs by hand.

Modification date

For this one, we get definitely the correct year, however, by updating the headers to the actual year, we add an modification in this year. In the end, we would need to update all files to this year (Except those, that weren't touch outsider their initial creation year.)

Options

In the end it boils down to 3 options with their ups and downs

🅰 git_add_date to today (current proposal)

This is an approach to get the most accurate numbers out.
Pros:

  • Sophisticated
  • We get a feeling, how old the code might be

Cons:

  • The first year can be wrong (too recent) due to git-bugs
  • Needs code for handling

🅱 existing_date to today

Similar to 🅰 but with the assumption, that we trust our old code more than the buggy git-log.

🅲 min(🅰, 🅱)

We could use the data provided in 🅰 and 🅱 and only use the git_add_date when it is smaller, than the indicated date. Still not perfect, but maybe a bit better. This option is probably not the best for keeping in the CI and pre-commit.

🅳 2016 to today

Uses the full span.
Pros:

  • Check is easy
  • Every file looks the same

Cons:

  • Header contains no additional information
  • We are pre-dating all (c)s

Other changes

  • Acts -> ACTS, since we converged to that spelling
  • http:// -> https:// in the license link

@AJPfleger AJPfleger added this to the next milestone Sep 17, 2024
@github-actions github-actions bot added Component - Core Affects the Core module Infrastructure Changes to build tools, continous integration, ... Component - Fatras Affects the Fatras module Component - Examples Affects the Examples module Component - Plugins Affects one or more Plugins Component - Documentation Affects the documentation Event Data Model Clustering SP formation Vertexing Seeding Track Finding labels Sep 17, 2024
@CarloVarni
Copy link
Collaborator

Hi @AJPfleger I'm afraid you have to update this to include the latest PRs that went into ACTS

@AJPfleger
Copy link
Contributor Author

@CarloVarni I covered all new files in my daily update.

andiwand
andiwand previously approved these changes Oct 1, 2024
View reviewed changes
Copy link
Contributor

@andiwand andiwand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's get this in

@paulgessinger
Copy link
Member

paulgessinger commented Oct 1, 2024 •
edited
Loading

I'm sorry to delay this, but does this mean we need to touch every single file every 1st of January or the CI is red?

I think if we update this, the solution needs to be:

  1. In pre-commit, update files that are part of the staged changes
  2. In PRs, only run the license check on files that are changed in the PR. Something like this could work here.

@andiwand
Copy link
Contributor

andiwand commented Oct 1, 2024

Oh right I forgot about that issue. A few potential solutions

  1. don't care about the year at all - that brings us back to the existing solution?
  2. validate and constrain the start and end year
  3. only put 2016
  4. only verify the end date if the file changed

@AJPfleger
Copy link
Contributor Author

I would prefer most a static solution in the form of
// Copyright (C) 2016 CERN for the benefit of the ACTS project
or
// Copyright (C) 2016- CERN for the benefit of the ACTS project
to symbolise, that the project is still ongoing.

Is the first solution fine for everyone?

@CarloVarni
Copy link
Collaborator

I would prefer most a static solution in the form of // Copyright (C) 2016 CERN for the benefit of the ACTS project or // Copyright (C) 2016- CERN for the benefit of the ACTS project to symbolise, that the project is still ongoing.

Is the first solution fine for everyone?

The first one would make people think they have to update the year when they change a file. The second one seems uncomplete with a dangling - imho

@andiwand
Copy link
Contributor

andiwand commented Oct 1, 2024

I am also in favor of this

// Copyright (C) 2016 CERN for the benefit of the ACTS project

It also makes clear that this is the first date of the copyright, whatever it means, but it cannot be confused anymore with the date of the file which is a good thing IMO

@AJPfleger
Copy link
Contributor Author

In today's developer meeting, no-one disfavoured to use the static (C) 2016 approach

@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Oct 1, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@kodiakhq kodiakhq bot merged commit 11be339 into acts-project:main Oct 1, 2024
42 checks passed
@github-actions github-actions bot removed the automerge label Oct 1, 2024
@acts-project-service
Copy link
Collaborator

✅ Athena integration test results [11be339]

✅ All tests successful

status job report
🟢 report_pull_request
🟢 run_art_test: test_data18_13TeV_1000evt
🟢 run_art_test: test_ttbarPU40_reco
🟢 run_workflow_tests_run4_mc
🟢 run_workflow_tests_run2_mc
🟢 run_workflow_tests_run2_data
🟢 run_workflow_tests_run3_mc
🟢 run_workflow_tests_run3_data
🟢 test_ActsDumpGeometryIdentifiers
🟢 test_ActsCheckObjectCountsCachedWorkflow
🟢 test_ActsCheckObjectCountsWorkflow
🟢 test_ActsEFTrackFit
🟢 test_ActsPersistifySeeds
🟢 test_ActsBenchmarkWithSpot
🟢 test_ActsAnalogueClustering
🟢 test_ActsWorkflowHeavyIons
🟢 test_ActsWorkflowFastTracking
🟢 test_ActsWorkflowCached
🟢 test_ActsWorkflow
🟢 test_ActsValidateAmbiguityResolution
🟢 test_ActsValidateResolvedTracks
🟢 test_ActsValidateTracks
🟢 test_ActsValidateActsCoreSpacePoints
🟢 test_ActsValidateActsSpacePoints
🟢 test_ActsValidateSeeds
🟢 test_ActsValidateOrthogonalSeeds
🟢 test_ActsValidateClusters
🟢 test_ActsPersistifyEDM
🟢 test_ActsGSFRefitting
🟢 test_ActsKfRefitting
🟢 test_ActsExtrapolationAlgTest
🟢 test_ActsITkTest
🟢 run_unit_tests

@AJPfleger AJPfleger deleted the license branch October 2, 2024 06:38
@paulgessinger paulgessinger modified the milestones: next, v37.0.0 Oct 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andiwand andiwand andiwand approved these changes

Assignees
No one assigned
Labels
Ambiguity Resolution Clustering Component - Core Affects the Core module Component - Examples Affects the Examples module Component - Fatras Affects the Fatras module Component - Plugins Affects one or more Plugins Event Data Model Infrastructure Changes to build tools, continous integration, ... Seeding SP formation Track Finding Track Fitting Vertexing
Projects
None yet
Milestone
v37.0.0
Development

Successfully merging this pull request may close these issues.
5 participants
@AJPfleger @CarloVarni @paulgessinger @andiwand @acts-project-service